tests/integration/test_evaluation_data_proposal.yaml (1)

95-104: 💤 Low value

expected_response is likely unused for this metric.

custom:proposal_evaluation_correctness is a turn-level LLM-as-judge metric that requires only response (no ground truth), so expected_response here will not be consulted during scoring. It's harmless but can mislead readers into thinking the judge compares against it. Consider dropping it or adding a brief comment clarifying it's documentation-only.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/test_evaluation_data_proposal.yaml` around lines 95 - 104,
The test includes an unnecessary expected_response alongside the turn-level
judge metric custom:proposal_evaluation_correctness (defined in turn_metrics and
turn_metrics_metadata) which doesn't use ground truth; remove the
expected_response block from the test or, if you want to keep it for
human-readable documentation, add a short inline comment next to
expected_response stating it is documentation-only and not used by the
custom:proposal_evaluation_correctness metric so readers aren’t misled.

tests/integration/test_proposal_evaluation.py (1)

204-236: ⚡ Quick win

Test asserts less than its docstring claims.

The docstring states this verifies that custom:proposal_evaluation_correctness runs against the response and the pipeline completes, but the body only checks that turn.response is populated — identical to test_full_lifecycle's response check. Nothing confirms the judge metric actually produced a result.

Since live LLM scores are nondeterministic, asserting a specific score is fragile, but you can confirm the metric path executed by checking the emitted results (e.g., the JSON output written to tmp_path / "eval_output") contain a custom:proposal_evaluation_correctness entry for the turn. This makes the test meaningfully distinct from the lifecycle test.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/test_proposal_evaluation.py` around lines 204 - 236, The
test_judge_evaluation currently only asserts that ProposalDriver populated
turn.response; update it to also verify that the judge metric ran by reading the
evaluation output written to the configured storage (the FileBackend output_dir
set to tmp_path / "eval_output") after calling evaluate(system_config,
eval_data) and assert that a result entry for
custom:proposal_evaluation_correctness exists and is associated with the
evaluated turn; locate this logic near the test_judge_evaluation function and
use the same identifiers (system_config, evaluate, eval_data, tmp_path, and the
turn from eval_data[0].turns[0]) to load the JSON results and assert the
presence of the custom:proposal_evaluation_correctness metric for that turn.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/integration/agentic/scripts/_setup_infra-openai.sh (1)

34-44: 💤 Low value

Optional: align secret creation with the safer oc create secret --dry-run pattern.

Interpolating ${OPENAI_API_KEY} directly into the heredoc YAML can break if the value ever contains YAML-special characters. The Claude/Vertex infra script already uses oc create secret … --dry-run=client -o yaml | oc apply -f -, which sidesteps escaping. Consider using the same approach here for consistency and robustness.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/agentic/scripts/_setup_infra-openai.sh` around lines 34 -
44, Replace the heredoc that directly interpolates ${OPENAI_API_KEY} with the
safer oc create secret --dry-run=client -o yaml pattern: instead of the cat
<<EOF | oc apply ... block that creates the Secret named eval-llm-credentials,
call oc create secret generic eval-llm-credentials with --from-literal entries
for OPENAI_API_KEY, LIGHTSPEED_AGENT_PROVIDER and OPENAI_MODEL (using the
AGENT_MODEL and OPENAI_API_KEY vars), add --dry-run=client -o yaml, and pipe
that output into oc apply -n "$OPERATOR_NS" -f -; update the script to remove
the heredoc and use this create|apply pipeline so values containing YAML-special
chars are correctly handled.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/lightspeed_evaluation/pipeline/evaluation/driver.py (1)

143-148: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Honor enabled=False before creating cluster dependencies.

validate_config() explicitly skips the CLI presence check when the driver is disabled, but this constructor still eagerly builds KubeCLI and ProposalAmender. That leaves ProposalDriver inconsistent with HttpApiDriver: a disabled proposal driver can still reach runtime code paths that try to use an empty CLI path instead of behaving like a no-op.

Proposed fix

 class ProposalDriver(AgentDriver):
     """Driver that manages Proposal CR lifecycle via oc/kubectl CLI."""
 
     def __init__(self, config: dict[str, Any], *, enabled: bool = True) -> None:
         """Initialize the proposal driver."""
         super().__init__(config, enabled=enabled)
         self._cli = self._resolve_cli()
-        self._kube_cli = KubeCLI(
-            cli_path=self._cli,
-            namespace=self._config.namespace,
-            timeout=self._config.cli_timeout,
-        )
-        self._amender = ProposalAmender(self._kube_cli)
+        self._kube_cli = None
+        self._amender = None
+        if enabled:
+            self._kube_cli = KubeCLI(
+                cli_path=self._cli,
+                namespace=self._config.namespace,
+                timeout=self._config.cli_timeout,
+            )
+            self._amender = ProposalAmender(self._kube_cli)
@@
     def execute_turn(
         self, turn_data: TurnData, conversation_id: Optional[str] = None
     ) -> tuple[Optional[str], Optional[str]]:
         """Execute a Proposal CR lifecycle for a single turn."""
+        if not self._enabled or self._kube_cli is None or self._amender is None:
+            return None, conversation_id
+
         # Proposal CR lifecycle:

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lightspeed_evaluation/pipeline/evaluation/driver.py` around lines 143 -
148, The constructor currently always instantiates KubeCLI and ProposalAmender
even when the driver is disabled; change the ProposalDriver constructor to only
create self._kube_cli = KubeCLI(...) and self._amender = ProposalAmender(...)
when the driver is enabled (e.g., if self._config.enabled: ...), otherwise set
those attributes to None (or leave uninitialized) so no cluster dependencies are
created for a disabled driver, and update any methods that reference
self._kube_cli or self._amender to guard for None or perform lazy
initialization.

tests/unit/pipeline/evaluation/test_proposal_driver.py (1)

624-625: ⚡ Quick win

Assert denied turns still produce a response.

Denied is now a non-error path, so downstream evaluation will still consume the amended summary. This test only checks proposal_status, which would miss regressions where denied turns stop populating turn.response.

Proposed test addition

         error, _ = driver.execute_turn(turn)
 
         assert error is None
+        assert turn.response
         assert turn.proposal_status == status

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/pipeline/evaluation/test_proposal_driver.py` around lines 624 -
625, The test currently only asserts error is None and turn.proposal_status ==
status, but when status is Denied we must also ensure the turn still produces a
response; update the test in test_proposal_driver.py to assert that
turn.response is populated for denied turns (e.g., assert turn.response is not
None and/or assert turn.response.text or length > 0) so regressions that drop
the amended summary are caught; reference the existing locals error, turn, and
proposal_status/status when adding the extra assertion(s).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/integration/agentic/scripts/setup_crashloop_probe-claude-vertex.sh (1)

13-17: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Poll for the actual CrashLoopBackOff state instead of swallowing the wait failure.

oc wait --for=condition=Available=false ... || true followed by a fixed sleep 10 masks timeout failures and can return success before crashloop-probe-demo has actually entered CrashLoopBackOff. Downstream processing treats exit 0 as "scenario ready," so the proposal scenario can run against a not-yet-failing workload. This is the same weakness that was addressed in setup_oomkill-openai.sh by actively polling pod container state; please apply the equivalent here.

🔧 Suggested fix

-echo "Waiting for crashloop-probe-demo pod to appear..."
-oc wait --for=condition=Available=false deployment/crashloop-probe-demo \
-  -n "$TEST_NS" --timeout=60s 2>/dev/null || true
-sleep 10
-echo "Setup complete."
+echo "Waiting for crashloop-probe-demo pod to reach CrashLoopBackOff..."
+TIMEOUT=120
+INTERVAL=5
+ELAPSED=0
+while [ "$ELAPSED" -lt "$TIMEOUT" ]; do
+  WAITING_REASON=$(oc get pod -l app=crashloop-probe-demo -n "$TEST_NS" \
+    -o jsonpath='{.items[0].status.containerStatuses[0].state.waiting.reason}' 2>/dev/null || true)
+  if [ "$WAITING_REASON" = "CrashLoopBackOff" ]; then
+    echo "Pod reached expected state (waiting=$WAITING_REASON)."
+    echo "Setup complete."
+    exit 0
+  fi
+  sleep "$INTERVAL"
+  ELAPSED=$((ELAPSED + INTERVAL))
+done
+echo "ERROR: Timed out after ${TIMEOUT}s waiting for CrashLoopBackOff." >&2
+exit 1

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration/agentic/scripts/setup_crashloop_probe-claude-vertex.sh`
around lines 13 - 17, Replace the current blind wait in
setup_crashloop_probe-claude-vertex.sh that uses "oc wait
--for=condition=Available=false deployment/crashloop-probe-demo ... || true" and
a fixed sleep with an active poll that checks the pod's container state until it
reaches CrashLoopBackOff (or times out); specifically, loop (with a configurable
timeout and short sleep interval) using oc get pods (or oc get pod -l
app=crashloop-probe-demo) and inspect the
pod.status.containerStatuses[*].state.waiting.reason for "CrashLoopBackOff" for
the crashloop-probe-demo pod in $TEST_NS, and fail the script if the timeout
expires — mirror the polling approach used in setup_oomkill-openai.sh but target
the crashloop-probe-demo pod/container.

src/lightspeed_evaluation/pipeline/evaluation/cli.py (3)

12-50: 💤 Low value

Consider completing Google-style docstrings with Args sections.

The public API methods have basic docstrings but are missing Args sections. As per coding guidelines, public APIs in src/**/*.py should use Google-style docstrings, which typically include Args, Returns, and Raises sections.

Example for run():

def run(
    self,
    args: list[str],
    stdin: Optional[str] = None,
) -> subprocess.CompletedProcess[str]:
    """Run a CLI command and return the completed process.
    
    Args:
        args: Command arguments to pass to the CLI binary.
        stdin: Optional string to pass to the command's stdin.
    
    Returns:
        A CompletedProcess instance with captured stdout/stderr.
    """

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lightspeed_evaluation/pipeline/evaluation/cli.py` around lines 12 - 50,
The docstrings on the public interface CLIClient methods (__init__, run,
get_resource, apply, delete) are missing Google-style sections; update each
method's docstring to include Args (describe parameters like timeout, args,
stdin, resource_plural, name, manifest), Returns (describe return types such as
subprocess.CompletedProcess[str] or tuple[dict[str, Any], Optional[str]]), and
Raises if applicable (e.g., any exceptions your implementations may propagate),
following the Google docstring format used across the repo so tools and readers
can parse parameter and return semantics consistently.

---

61-84: ⚡ Quick win

Add structured logging for timeout events.

The timeout handling correctly converts subprocess.TimeoutExpired into a failed CompletedProcess, but timeout events are not logged. As per coding guidelines, code in src/lightspeed_evaluation/**/*.py should use structured logging with appropriate log levels. Logging timeouts at this layer would improve observability for infrastructure issues.

📊 Suggested enhancement

+import logging
+
+logger = logging.getLogger(__name__)
+
+
 class CLIClient(ABC):

         except subprocess.TimeoutExpired:
+            logger.error(
+                "CLI command timed out after %ds: %s",
+                self._timeout,
+                " ".join([self._cli, *args]),
+            )
             cmd_str = " ".join([self._cli, *args])
             return subprocess.CompletedProcess(

As per coding guidelines: "Use structured logging with appropriate log levels in Python code" for src/lightspeed_evaluation/**/*.py.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lightspeed_evaluation/pipeline/evaluation/cli.py` around lines 61 - 84,
In the run method's except block for subprocess.TimeoutExpired, add structured
logging (use a module-level logger via logging.getLogger(__name__)) to record
the timeout event with fields like command (compose from self._cli and args),
timeout seconds (self._timeout), and exception info; log at warning or error
level before returning the subprocess.CompletedProcess, and ensure the module
imports logging and defines the logger variable so the log call can reference
it.

---

105-111: ⚡ Quick win

Add structured logging for JSON decode failures.

JSON decode failures indicate malformed output from the CLI, which is a significant issue worth logging at this layer. Currently, only the caller logs when it receives an error. As per coding guidelines, use structured logging in src/lightspeed_evaluation/**/*.py.

📊 Suggested enhancement

         try:
             return json.loads(result.stdout), None
         except json.JSONDecodeError as exc:
+            logger.error(
+                "Failed to parse JSON for %s/%s: %s. stdout: %r",
+                resource_plural,
+                name,
+                exc,
+                result.stdout[:200],
+            )
             return (
                 {},
                 f"Failed to parse JSON for {resource_plural}/{name}: {exc}",
             )

As per coding guidelines: "Use structured logging with appropriate log levels in Python code" for src/lightspeed_evaluation/**/*.py.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lightspeed_evaluation/pipeline/evaluation/cli.py` around lines 105 - 111,
Add structured logging in the JSON parse error branch so malformed CLI output is
logged with context before returning the error: catch json.JSONDecodeError in
the block that currently returns ({}, f"Failed to parse JSON for
{resource_plural}/{name}: {exc}") and call the module logger (the file's
module-level logger, e.g., logger or getLogger(__name__)) to emit an error-level
structured log containing fields resource_plural, name, stdout (or a truncated
version), and the exception/stacktrace, then return the same error tuple; update
imports if needed to ensure the logger is available.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes